Feat: Add version change indicators for Dag and bundle versions in Grid view#53216
Feat: Add version change indicators for Dag and bundle versions in Grid view#53216choo121600 wants to merge 6 commits intoapache:mainfrom
Conversation
09ecc2b to
bd021e8
Compare
There was a problem hiding this comment.
Thanks for the PR! I think this is bringing back the deprecated and removed monolith endpoint with its methods. I think we should integrate the fix into only new structure
cc: @dstandish @pierrejeambrun
|
@bugraoz93 Thanks for your quick feedback! I saw something strange while fixing the conflict, I think my endpoints follow the old style. I’ll change them to use the new structure. |
Amazing, thanks a lot! |
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/grid.py
Outdated
Show resolved
Hide resolved
|
I’ve updated the tests and removed the old structure, |
|
I’ve tested the scenarios I had in mind, and everything looks good so far 🙌 |
bugraoz93
left a comment
There was a problem hiding this comment.
Thanks for the changes! I believe this PR still contains old deprecated code :)
airflow-core/src/airflow/api_fastapi/core_api/services/ui/grid.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/services/ui/grid.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/services/ui/dag_version_service.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/services/ui/dag_version_service.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/services/ui/dag_version_service.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/services/ui/dag_version_service.py
Outdated
Show resolved
Hide resolved
|
I thought I deleted the old version, but it still there. |
|
If we want to visualize version change, in my opinion, we should do it for bundle version and not dag version that changes everytime especially for dynamic dags cc @jedcunningham |
In that case, we might need to use an icon and then show the bundle version in a tooltip. Maybe something like |
Maybe we need both with different colors, dag version change (to task level) + bundle version change (dag run level only). If we were to keep only one, I would be in favor of keeping the DagVersion because it is consistant with the Graph(Graph is showing structures of specific dag versions), And because I think source code is less important than what was actually executed. |
pierrejeambrun
left a comment
There was a problem hiding this comment.
I think the code looks very complicated for what we are trying to achieve. (And also there are some unused stuff I believe from the legacy grid)
The backend need to serialize the dag_run.versions in the GridRunsResponse, same for tasks.
And in the UI, we just need a simple logic to check if dag_run.version of Nth run is equal to dag_run.version of (N-1)th run.
(Same logic goes for tasks, and yes if DagRun.dag_versions.lenght === 1 no need to go try the task level logic)
Maybe I missed something though.
airflow-core/src/airflow/ui/src/layouts/Details/Grid/TaskInstancesColumn.tsx
Outdated
Show resolved
Hide resolved
I'm afraid that we can potentially have as many tooltips as TI in the grid. Which would most likely bring back tooltip performance issue we observed in the past. |
bbovenzi
left a comment
There was a problem hiding this comment.
I think we should add a legend to make it easier for users to learn dag versions vs bundle version changes.
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/layouts/Details/PanelButtons.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/layouts/Details/Grid/TaskInstancesColumn.tsx
Outdated
Show resolved
Hide resolved
|
@bbovenzi I’ve been thinking about the legend, and currently, the horizontal indicator for Dag version changes and the indicator for bundle version changes use the same icon. Here’s an example of what it looks like. What do you think?
|
@bbovenzi @pierrejeambrun This part hasn’t been implemented yet. I’d like to hear your ideas regarding the legend. |
pierrejeambrun
left a comment
There was a problem hiding this comment.
Nice, looking better and better.
A few suggestions to try to make things simpler and improve UX.
airflow-core/src/airflow/ui/src/constants/showVersionIndicatorOptions.ts
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/common.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/datamodels/ui/common.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/layouts/Details/PanelButtons.tsx
Outdated
Show resolved
Hide resolved
pierrejeambrun
left a comment
There was a problem hiding this comment.
Feel free to mark addressed comments as resolved.
Let me know when this needs another round of review.
airflow-core/src/airflow/ui/src/constants/showVersionIndicatorOptions.ts
Outdated
Show resolved
Hide resolved
|
Except for this part #53216 (comment) |
|
We'll need to rebase this after virtualizing the grid's vertical scroll. |
fad0cf6 to
f48e7ec
Compare
|
Hi, this feature looks really cool. Could you please help resolve the conflicts? Thanks! |
Oops, I missed the notification earlier 😅 |
guan404ming
left a comment
There was a problem hiding this comment.
Look nice, just left some comments. Thanks!
airflow-core/src/airflow/ui/src/constants/showVersionIndicatorOptions.ts
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/components/ui/VersionIndicator.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/ui/src/layouts/Details/PanelButtons.tsx
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/services/ui/grid.py
Outdated
Show resolved
Hide resolved
c600518 to
fdfccb9
Compare
guan404ming
left a comment
There was a problem hiding this comment.
UI wise looks nice thanks!
Left one nit.
pierrejeambrun
left a comment
There was a problem hiding this comment.
Nice, we're getting closer! Thanks for your continuous effort and being patient, that's a complex feature and that's why there's a bunch of back and forth.
There are a lot of useMemo, I think those are not needed anymore with react-compiler
| run_after: datetime | ||
| state: DagRunState | None | ||
| run_type: DagRunType | ||
| bundle_version: str | None = None |
| # get the highest dag_version_number from TIs for each run | ||
| latest_ti_version = ( | ||
| select( | ||
| TaskInstance.run_id, | ||
| func.max(DagVersion.version_number).label("version_number"), | ||
| ) | ||
| .join(DagVersion, TaskInstance.dag_version_id == DagVersion.id) | ||
| .where(TaskInstance.dag_id == dag_id) | ||
| .group_by(TaskInstance.run_id) | ||
| .subquery() | ||
| ) |
There was a problem hiding this comment.
We don't need to go through all that trouble to only retrieve the 'last' version. Just return the DagRun.dag_versions attribute.
Also this will be consistent for other contributor. People will get confused as to why a DagRun has dag_versions as a list in the plublic interface, but on the grid endpoint here it's only dag_version and not a list anymore.
I find this confusing and it adds a lot of unecessary application code.
| const taskInstances = useMemo( | ||
| () => gridTISummaries?.task_instances ?? [], | ||
| [gridTISummaries?.task_instances], | ||
| ); | ||
| const taskInstanceMap = useMemo(() => { | ||
| const map = new Map<string, LightGridTaskInstanceSummary>(); | ||
|
|
||
| for (const ti of taskInstances) { | ||
| map.set(ti.task_id, ti); | ||
| } | ||
|
|
||
| return map; | ||
| }, [taskInstances]); |
There was a problem hiding this comment.
I thought react compiler would memo this for us.
There was a problem hiding this comment.
Correct. Let's remove them.
| } | ||
|
|
||
| return gridRuns.map((run, index) => { | ||
| const prevRun = gridRuns[index + 1]; |
There was a problem hiding this comment.
| const prevRun = gridRuns[index + 1]; | |
| const nextRun = gridRuns[index + 1]; |
| BUNDLE = "bundle", | ||
| DAG = "dag", |
There was a problem hiding this comment.
Nit:
| BUNDLE = "bundle", | |
| DAG = "dag", | |
| BUNDLE_VERSION = "bundle", | |
| DAG_VERSION = "dag", |
| let hasVersionChangeFlag = false; | ||
|
|
||
| if ( | ||
| hasMixedVersions && | ||
| (showVersionIndicatorMode === VersionIndicatorDisplayOptions.DAG || | ||
| showVersionIndicatorMode === VersionIndicatorDisplayOptions.ALL) && | ||
| idx > 0 | ||
| ) { | ||
| const prevVirtualItem = itemsToRender[idx - 1]; | ||
| const prevNode = prevVirtualItem ? nodes[prevVirtualItem.index] : undefined; | ||
| const prevTaskInstance = prevNode ? taskInstanceMap.get(prevNode.id) : undefined; | ||
|
|
||
| hasVersionChangeFlag = Boolean( | ||
| prevTaskInstance && prevTaskInstance.dag_version_number !== taskInstance.dag_version_number, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why aren't we checking for BundleVersion change too ?
DagVersion could be the same (serialized dag is identical) but the bundle version changed (version of the file in the code)
There was a problem hiding this comment.
If we only use this in the grid view then we should move it to layouts/Details/Grid instead. components are for general reshareable components. (/ui was also supposed to be for wrappers that weren't included in chakra-ui but that we hadn't customized, but we haven't been enforcing that too well)
| // Global setting: applies to all Dags (intentionally not scoped to dagId) | ||
| const [showVersionIndicatorMode, setShowVersionIndicatorMode] = | ||
| useLocalStorage<VersionIndicatorDisplayOptions>( | ||
| `version_indicator_display_mode`, |
There was a problem hiding this comment.
I wonder if we should make a src/constants/localStorage.ts file to keep track of how many localStorage values we have.
| */ | ||
| import { createListCollection } from "@chakra-ui/react"; | ||
|
|
||
| export enum VersionIndicatorDisplayOptions { |
There was a problem hiding this comment.
VersionIndicatorOptions or VersionDisplayOptions
We use this a lot. Let's try to have a simpler variable name.


Summary
Adds visual indicators in the Grid view to help users identify Dag version changes and detect mixed-version TaskInstances within a single DagRun.
Problem
Solution
This PR introduces visual cues in the Grid view to address these pain points:
Change
Backend
Frontend
Screenshots
Version Change
LocalDagBundle
Dag Version Indicator Tooltip(vertical)

Dag Version Indicator Tooltip(horizontal)

GitDagBundle
Dag Version Indicator Tooltip

Bundle Version Indicator Tooltip

Dark Mode
Toggle Option(with Legend)
Users can choose 'Show All', 'Show Dag Version', 'Show Bundle Version', 'Hide All'
Show All
Show Bundle Version
Show Dag Version
Hide All
Indicator Scenarios
Scenario 1: Normal
Scenario 2: Version Change
Scenario 3: Mixed Version
Scenario 4: Bundle Version & DagVersion Change
Scenario 5: Only Bundle Change (Dynamic Dag)
closes: #52286
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.